Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: types.HeaderHooks for RLP overrides #89

Merged
merged 8 commits into from
Dec 17, 2024
Merged

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Dec 12, 2024

Why this should be merged

The types.Header fields of both coreth and subnet-evm have been modified such that their RLP encodings (i.e. block hashes) aren't compatible with vanilla geth nor each other. This PR adds support for arbitrary RLP encoding coupled with type-safe extra payloads.

How this works

Equivalent to #1 (params) and #44 (types.StateAccount) registration of pseudo-generic payloads. The only major difference is the guarantee of a non-nil payload pointer, which means that the payload hooks are never called on nil pointers as this would make it difficult to decode RLP into them.

How this was tested

Round-trip RLP {en,de}coding via a registered stub hook.

@ARR4N ARR4N mentioned this pull request Dec 12, 2024
@ARR4N ARR4N marked this pull request as ready for review December 12, 2024 15:01
@ARR4N ARR4N requested review from a team, darioush, qdm12 and michaelkaplan13 and removed request for a team December 12, 2024 15:01
core/types/block.libevm_test.go Show resolved Hide resolved
defer TestOnlyClearRegisteredExtras()

extras := RegisterExtras[stubHeaderHooks, *stubHeaderHooks, struct{}]()
rng := ethtest.NewPseudoRand(13579)
Copy link
Collaborator

@qdm12 qdm12 Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason for 13579? Or just smashed digits on the keyboard? ⌨️ I feel we should really just seed everything with 0 - if we want to test with various random values, we should fuzz instead 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we should really just seed everything with 0

If there's a particular reason then I'm happy to consider it as the only reason I choose numbers is for some fun. Practically I think it's all the same, no?

Copy link
Collaborator

@qdm12 qdm12 Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look like a magic number although it's not, at least with 0 it does look like a this-number-does-not-matter-see-I-m-using-zero. My mind might had been trained too much by the https://github.com/tommy-muehle/go-mnd linter to be fair 😄 What do you think?

Maybe we could address those in a chore PR later finding all numbers with a regex, I can create a super low priority issue for this 😸

core/types/block.libevm.go Outdated Show resolved Hide resolved
core/types/rlp_payload.libevm.go Show resolved Hide resolved
core/types/block.libevm_test.go Outdated Show resolved Hide resolved
core/types/block.libevm_test.go Outdated Show resolved Hide resolved
core/types/block.libevm.go Show resolved Hide resolved
core/types/block.libevm.go Outdated Show resolved Hide resolved
core/types/block.libevm.go Show resolved Hide resolved
@ARR4N ARR4N merged commit dc7e27a into main Dec 17, 2024
4 checks passed
@ARR4N ARR4N deleted the arr4n/header-rlp-hooks branch December 17, 2024 13:55
ARR4N added a commit that referenced this pull request Dec 19, 2024
## Why this should be merged

JSON equivalent of #89.

## How this works

The check for registered extras, previously used in `{En,De}codeRLP()`
methods is abstracted into a `Header.hooks() HeaderHooks` method that
either returns (a) an instance of the registered type or (b) a
`NOOPHeaderHooks` if no registration was performed. This is then used
for all hooks, new (JSON) and old (RLP).

## How this was tested

Extension of existing unit tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants